Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add cds logger #93

Merged
merged 9 commits into from
Nov 15, 2024
Merged

Conversation

aramovic79
Copy link
Member

No description provided.

@aramovic79 aramovic79 linked an issue Nov 12, 2024 that may be closed by this pull request
@aramovic79
Copy link
Member Author

aramovic79 commented Nov 13, 2024

@zongqichen @swennemers @Fannon : A logging is added to the ORD Plugin. For logging is re-used the logger @sap/cds.
I thought it's sufficient to have only two levels of logging: DEBUG and WARN(including ERROR); the first one for development environment and the second one(s) for production. "Activating" logging in DEBUG mode can be done in two ways: by setting environment variable DEBUG or by setting the DEBUG property in the .cdsrc.json to some truthy value.
Could you please review the changes? Thanks.

@aramovic79 aramovic79 marked this pull request as ready for review November 13, 2024 16:45
@aramovic79 aramovic79 self-assigned this Nov 13, 2024
@zongqichen
Copy link
Contributor

@zongqichen @swennemers @Fannon : A logging is added to the ORD Pluging. For logging is re-used the logger @sap/cds. I thought it's sufficient to have only two levels of logging: DEBUG and WARN(including ERROR); the first one for development environment and the second one(s) for production. "Activating" logging in DEBUG mode can be done in two ways: by setting environment variable DEBUG or by setting the DEBUG property in the .cdsrc.json to some truthy value. Could you please review the changes? Thanks.

Hi @aramovic79 , I agree to have only two levels, but the levels are the others. My opinion is level INFO and ERROR are important. When you debug or monitor logs in production env, INFO and ERROR can provide clear message to people who look into it. When I debug in dev env, the DEBUG level logger can't support me more than debug mode in vscode. Based on my experience, I always ignore warning message, since for me the program is either succeed or failed. So we need ERROR level message which can wake up admin at night shift when the program broke out.

@aramovic79
Copy link
Member Author

aramovic79 commented Nov 14, 2024

Hi @aramovic79 , I agree to have only two levels, but the levels are the others. My opinion is level INFO and ERROR are important. When you debug or monitor logs in production env, INFO and ERROR can provide clear message to people who look into it. When I debug in dev env, the DEBUG level logger can't support me more than debug mode in vscode. Based on my experience, I always ignore warning message, since for me the program is either succeed or failed. So we need ERROR level message which can wake up admin at night shift when the program broke out.

Please check here how the log levels are enumerated in cap cds(the lower number => the higher severity).

This is here-implemented logging setup:

In dev environment, the DEBUG level is set to some truthy value(in .cdsrc.json or by setting DEBUG env. variable), which results in showing in console(or log file or ...) all of the following logs:

  • logger.DEBUG('Will be shown');
  • logger.INFO('Will be shown');
  • logger.WARN('Will be shown');
  • logger.ERROR('Will be shown');

In production, the default level is WARNING, which means that only the following logs will be shown:

  • logger.WARN('Will be shown');
  • logger.ERROR('Will be shown');

@zongqichen : Is your proposal to set default level in production to be ERROR(so to "consider" only the logger.ERROR('Will be shown);)? If yes, I'm fine with that ...

@zongqichen
Copy link
Contributor

Hi @aramovic79 , I agree to have only two levels, but the levels are the others. My opinion is level INFO and ERROR are important. When you debug or monitor logs in production env, INFO and ERROR can provide clear message to people who look into it. When I debug in dev env, the DEBUG level logger can't support me more than debug mode in vscode. Based on my experience, I always ignore warning message, since for me the program is either succeed or failed. So we need ERROR level message which can wake up admin at night shift when the program broke out.

This is the current setup:

In dev environment, the DEBUG level is set to some truthy value(in .cdsrc.json or by setting DEBUG env. variable), which results in showing all of the following logs:

  • logger.**DEBUG**('Will be shown');
    
  • logger.**INFO**('Will be shown');
    
  • logger.**WARN**('Will be shown');
    
  • logger.**ERROR**('Will be shown');
    

In production, the default level is WARNING, which means that only the following logs will be shown:

  • logger.**WARN**('Will be shown');
    
  • logger.**ERROR**('Will be shown');
    

@zongqichen : Is your proposal to set default level in production to be ERROR(so to "consider" only the logger.ERROR('Will be shown);)? If yes, I'm fine with that ...

My proposal is even simpler. We don't need to distinguish dev or production environment, we only use INFO and ERROR when it is really necessary. It will keep our code and console clean. And I visit other repos in cap-js, there are quite mix, some repo use debug and warn like cds-typer, some don't use at all like change-tracking. My preference is the second one. However, I don't strong opinion on it, what do you think @swennemers @Fannon ?

@zongqichen zongqichen removed the request for review from Fannon November 14, 2024 09:50
@zongqichen
Copy link
Contributor

Hi @aramovic79 , I forget to mention it, could you revisit all codes and replace all, console.error, console.info or new Error with logger? If you think it's out of the scope, you could raise an new issue for it. Thx

@aramovic79 aramovic79 linked an issue Nov 14, 2024 that may be closed by this pull request
@aramovic79
Copy link
Member Author

aramovic79 commented Nov 14, 2024

Hi @aramovic79 , I forget to mention it, could you revisit all codes and replace all, console.error, console.info or new Error with logger? If you think it's out of the scope, you could raise an new issue for it. Thx

Done as requested. Also, the issue #86 is fixed in this PR as well, since it was partially related to the logging. Could you please take a look? Thank you!

@zongqichen
Copy link
Contributor

Hi @aramovic79 , thank you for replacing console log. There is still a small issue need to be adjust, https://cap.cloud.sap/docs/node.js/cds-log#recommendations. The construct format message in logger will be expensive for production env. The best practice is always use , to separate.

@aramovic79
Copy link
Member Author

g. There is still a small issue need to be adjust, https://cap.cloud.sap/docs/node.js/cds-log#recommendations. The construct format message in logger will be expensive for production env. The best practice is always use , to separate.

Good point. Changed as proposed.

Copy link
Contributor

@zongqichen zongqichen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thx, it looks way better now

@aramovic79 aramovic79 merged commit b4faec2 into main Nov 15, 2024
3 checks passed
@aramovic79 aramovic79 deleted the feat/66-feat-add-logger-to-handle-errors branch November 15, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix: add error handling when custom.ord.json doesn't exist Feat: add logger to handle errors
2 participants